Extend the API of the validation registry for more performant custom validations #1614
Conversation
|
|
||
| getChecksAfter(): ValidationPreparation[] { | ||
| return this.entriesAfter; | ||
| } |
There was a problem hiding this comment.
I'd declare the properties as public readonly rather than using such getter methods.
There was a problem hiding this comment.
The problem with public properties is, that users could register checks directly in this property and without calling registerBeforeDocument(...), which bypasses our internal handling of exceptions.
Therefore I decided to keep the "simple getters".
There was a problem hiding this comment.
The getter does not change that: someone could still push elements to the returned array. If you really want to prevent that, you'd need to clone the array. But an easier and faster solution is just to document the property/method saying you shouldn't manipulate the array.
If you want to keep a getter, I suggest using a get property:
get checksAfter(): ValidationPreparation[] {
return this.entriesAfter;
}This is about code style: methods like getChecksAfter() are common in Java, while in JS/TS either value properties or get properties are used.
There was a problem hiding this comment.
someone could still push elements to the returned array. If you really want to prevent that, you'd need to clone the array.
You are right.
I replaced getChecksBefore() by get checksBefore(), since it is important to have a unified code style.
Beyond that, I personally prefer to have the new getChecksBefore() and the already existing getChecks() in the same style, but I am fine with get ... as well.
JohannesMeierSE
force-pushed
the
extend-validation-registry
branch
from
f4fea8e
to
4b5d5e9
Compare
|
Thanks for the discussions in the Langium dev meeting and @spoenemann for your review! I just pushed the improvements. |
… before/after validating an AST
…idation, typed and simplified test cases
JohannesMeierSE
force-pushed
the
extend-validation-registry
branch
from
91ac637
to
91e2df5
Compare
|
@spoenemann I pushed the fix and rebased to solve the conflicts with |
|
FYI: We need this feature in Typir. Therefore we are looking forward to a Langium release with this feature! |
When writing custom validations, often it is required to stream the (whole or parts of the) AST inside the registered checks, e.g. for checking unique names or for identifying circles/loops in relationships between the AST nodes (calls, inheritance). For huge ASTs, this approach has the drawback, that the AST is traversed multiple times, which decreases performance.
This PR proposes to register additional "checks" which are executed once before or after validating an AST/Langium document. The idea is to exploit the AST traversal already done by the DocumentBuilder and to remember only relevant information during AstNode-specific checks. The collected information is evaluated once after all checks are done.
The expected benefit are faster validations by less AST traversals inside custom checks for big ASTs.
In order to demonstrate the new API, I wrote simple test cases to check unique fully-qualified names and implemented the validation twice, once with the existing API (and with
streamAllContents) and once with the new API (and withoutstreamAllContents).Aspects to consider during the review: